crates/git-remote-gitlawb/src/main.rs (1)

301-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicates post_pack_round's POST/status-check/forward logic.

Lines 313-334 re-implement almost exactly what post_pack_round (lines 502-533) does: build the signed POST, send, check status via http_error_message, read the response body, and forward it to stdout. Since post_pack_round already takes (client, post_url, service, signing_key, body, stdout) matching this call site's variables, the receive-pack path can just call it directly instead of duplicating the block.

♻️ Suggested refactor

     let mut request_body = Vec::new();
     stdin
         .read_to_end(&mut request_body)
         .context("reading receive-pack request")?;
     tracing::debug!("pack request: {} bytes from git", request_body.len());

     if request_body.is_empty() {
         // e.g., already up-to-date — nothing to send
         tracing::debug!("empty request body — skipping POST");
         return Ok(());
     }

-    tracing::debug!("POST {post_url} ({} bytes)", request_body.len());
-    let req = build_pack_post_request(&client, &post_url, service, &request_body, signing_key);
-
-    // Attach the body after signing so the pack bytes are moved, not cloned —
-    // packs can be large and the clone doubled peak memory on push.
-    let pack_resp = req
-        .body(request_body)
-        .send()
-        .with_context(|| format!("POST {post_url}"))?;
-
-    if !pack_resp.status().is_success() {
-        let status = pack_resp.status();
-        let body = read_error_body(pack_resp);
-        let path = format!("/{service}");
-        bail!("{}", http_error_message("POST", &path, status, &body, None));
-    }
-
-    let pack_bytes = pack_resp.bytes().context("reading pack response")?;
-    tracing::debug!("pack response: {} bytes from node", pack_bytes.len());
-
-    stdout.write_all(&pack_bytes)?;
-    stdout.flush()?;
-
-    Ok(())
+    post_pack_round(&client, &post_url, service, signing_key, request_body, &mut stdout)
 }

This keeps behavior identical (same log lines, same error path) while removing a divergence risk between the two copies.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/git-remote-gitlawb/src/main.rs` around lines 301 - 336, Replace the
duplicated POST, status-check, response-reading, and stdout-forwarding block
after the empty-body guard with a direct call to post_pack_round, passing
client, post_url, service, signing_key, request_body, and stdout. Preserve the
existing request-body logging and empty-body early return while reusing
post_pack_round for the shared behavior.

crates/git-remote-gitlawb/tests/real_git_fetch.rs (1)

354-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Temp repos leak on assertion failure.

cleanup(&[server, clone]) (lines 387, 429) only runs if every prior assert!/assert_eq! in the test succeeds. A failed assertion (e.g. completed, fsck, or the FETCH_HEAD equality checks) panics before cleanup, leaking the unique_dir()-created server/clone repos (each with a real object DB) under the OS temp dir. Shim already avoids this class of leak for its own thread/listener via Drop (lines 111-126); the repo directories don't get the same RAII treatment.

♻️ Wrap temp dirs in an RAII guard so they're removed even on panic

struct TempDir(PathBuf);

impl std::ops::Deref for TempDir {
    type Target = Path;
    fn deref(&self) -> &Path {
        &self.0
    }
}

impl Drop for TempDir {
    fn drop(&mut self) {
        let _ = std::fs::remove_dir_all(&self.0);
    }
}

Then have unique_dir/build_divergent_repos return TempDir and drop the explicit cleanup(...) calls at the end of each test.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/git-remote-gitlawb/tests/real_git_fetch.rs` around lines 354 - 436,
Introduce an RAII temporary-directory guard for paths created by unique_dir and
returned through build_divergent_repos, implementing path access and Drop to
remove each directory. Update both real_git_multi_round_fetch_completes and
real_git_withheld_shaped_first_post to use the guarded values and remove the
explicit cleanup calls, ensuring directories are deleted during assertion panics
as well as normal completion.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes